Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve JsonRPC plugin settings #2430

Merged
merged 8 commits into from
Nov 21, 2023
Merged

Improve JsonRPC plugin settings #2430

merged 8 commits into from
Nov 21, 2023

Conversation

Garulf
Copy link
Member

@Garulf Garulf commented Nov 18, 2023

Problem

  • JsonRPC plugins are unable to make settings changes unless they have a SettingsTemplate.yaml file.
  • Plugin's are unable to initiate new setting keys
  • Plugin settings are only saved to disk upon exit

Solution

  • Allow plugins without SettingsTemplate.yaml to change their settings
  • Remove logic restricting settings key creation
  • Save plugin settings to file when updated

@Garulf Garulf self-assigned this Nov 18, 2023
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Garulf Garulf changed the title Allow settings change without template file Improve JsonRPC plugin settings Nov 18, 2023
@Garulf Garulf requested review from taooceros and jjw24 November 18, 2023 23:58
@Garulf Garulf requested a review from taooceros November 19, 2023 04:16
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Garulf Garulf enabled auto-merge November 19, 2023 08:09
@github-actions GitHub Actions
Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (4)

hyperlink
linkbtn
Linq
SYSLIB

To accept these unrecognized words as correct, you could run the following commands

... in a clone of the [email protected]:Flow-Launcher/Flow.Launcher.git repository
on the Allow-SettingsChange-without-template-file branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/Flow-Launcher/Flow.Launcher/actions/runs/6919276522/attempts/1'

OR

To have the bot accept them for you, reply quoting the following line:
@check-spelling-bot apply updates.

Errors (3)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors Count
❌ ignored-expect-variant 6
ℹ️ no-newline-at-eof 1
ℹ️ non-alpha-in-dictionary 9

See ❌ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

taooceros
taooceros reviewed Nov 19, 2023
View reviewed changes
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs
}
Save();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry another thoughts that why didn't we keep this consistent with other plugins. Save only be called when manually triggered or flow being closed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess just personal preference. It feels off to not save to file on change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah ok

@Garulf Garulf merged commit 851df97 into dev Nov 21, 2023
2 checks passed
@Garulf Garulf deleted the Allow-SettingsChange-without-template-file branch November 21, 2023 21:50
@jjw24 jjw24 added the bug Something isn't working label Nov 22, 2023
@jjw24 jjw24 added this to the 1.17.0 milestone Nov 22, 2023
@jjw24 jjw24 mentioned this pull request Feb 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taooceros taooceros taooceros approved these changes

@jjw24 jjw24 Awaiting requested review from jjw24

Assignees

@Garulf Garulf

Labels
bug Something isn't working
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Garulf @taooceros @jjw24